-
Notifications
You must be signed in to change notification settings - Fork 37
feat: support a priority queue for dials #325
Conversation
87aa0c1
to
b00d6cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small suggestion, otherwise LGTM 👍
README.md
Outdated
|
||
- `peer`: can be an instance of [PeerInfo][], [PeerId][] or [multiaddr][] | ||
- `options`: Optional | ||
- `options.priority`: Number of the priority of the dial, defaults to 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we start with higher numbered priorities to leave some wiggle room in case there are other use cases in future? eg HIGH=10, LOW=20, and then in future maybe there's a MEDIUM=15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of anything now, but that doesn't mean there wont be :). I've added those as constants to the constants file, and am now referencing them for the options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ups, I did not see this review discussion and left a comment in this regard too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Only left 2 minor comments
README.md
Outdated
|
||
- `peer`: can be an instance of [PeerInfo][], [PeerId][] or [multiaddr][] | ||
- `options`: Optional | ||
- `options.priority`: Number of the priority of the dial, defaults to 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the acceptable range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since items currently go into 1 of 2 queues that execute by order of adds, I updated the docs to try to be more clear about what's going on. I think if we expand on that in the future or actually add priority to each of the queues we can lock down what's acceptable.
options = { useFSM: false, priority: 1, ...options } | ||
_dial({ peerInfo, protocol: null, options, callback }) | ||
} | ||
|
||
/** | ||
* Adds the dial request to the queue for the given `peerInfo` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docs for using higher priority here
This adds basic support for priorities. Currently, all priority 0 (highest) items will be run through the main dial queue and all priorities >= 1 will go through the cold call queue. This enables the proposed auto dial feature in libp2p, libp2p/js-libp2p#349, to create low priority dials on peer discovery.
Since some applications may dial to peers without specifying a protocol, such as dialing to a content provider discovered through the DHT, only detecting the presence of a protocol is not sufficient to determine priority.
switch.dialer.connect
creates a direct path for applications to use the cold call queue.required by libp2p/js-libp2p#349